More speed and less memory usage via code generation by nitram509 · Pull Request #2 · benlowry/node-geoip-native

nitram509 · 2012-11-04T23:57:41Z

Hi,

I've made some more progressive enhancements within my fork of 'node-geoip-native'.
The basic ideas:

generating JS code from CSV file in 'build phase' or manual
generated sources can be used via normal require statement
- thus theres no warmup needed anymore
- thus the memory usage drops heavily
split the IPs and Countries in two separate arrays,
- thus the memory usage drops heavily
using more compact version of binary search
fixed all tests with lookup errors (see unit tests)

In general, I've measured ca. 10% performance increase during lookup
and a drop from ca. 50MB to ca. 30MB heap usage.

Please, check out this branch 'code_generation' and tell me what you think.

Tanks in advance
Martin

FIX: country name contains a '\r' on windows

…tected, as unit tests are green now

using country-names list with index, which results in half the memory usage

…tand

…wait/warmup

Merge branch 'master' into code_generation Conflicts: geoip.js

pozirk · 2013-04-18T15:14:29Z

Hello Martin!
I have just realized, that geoip return incorrect results sometimes, so I will try your fork instead.
Thank you for sharing!
Also, latest ip database from MaxMind has different size: http://dev.maxmind.com/geoip/geolite

pozirk · 2013-04-18T15:28:06Z

Looks like your version returns incorrect results as well.
For example, check against ip: 63.155.159.123, should be US, but return Japan.

nitram509 · 2013-04-18T20:32:15Z

Jup, it seems there are still wrong lookups. Thanks for pointing this out.
I've added this 63.155.159.123 to the unit tests (lookuoTest.js) and try to fix it.

pozirk · 2013-04-18T20:46:41Z

Looks like it gives wrong results all the time. :)

Some examples:
"63.150.141.128","63.156.193.255","1066831232","1067237887","US","United States"
"63.156.194.0","63.156.195.255","1067237888","1067238399","JP","Japan"
63.155.159.123 returns Japan.

"93.32.0.0","93.71.255.255","1562378240","1564999679","IT","Italy"
"93.72.0.0","93.79.255.255","1564999680","1565523967","UA","Ukraine"
93.79.113.99 returns Italy.

"138.187.0.0","138.191.255.255","2327511040","2327838719","CH","Switzerland"
"138.192.0.0","138.193.255.255","2327838720","2327969791","US","United States"
138.188.103.143 return US.

"213.87.0.0","213.88.127.255","3579248640","3579346943","RU","Russian Federation"
"213.88.128.0","213.88.184.255","3579346944","3579361535","SE","Sweden"
213.87.133.200 returns Sweden.

… and combining this with more range/equation checks

nitram509 · 2013-04-18T21:38:33Z

Please, verify my last commit.
I figured out, what was wrong. The binary search algorithm, I've implemented before assumed, that the target_ip exists within all records. But thats simply not the case. Thus I've switched to use another binary search approach ('Deferred detection of equality'). This way, the midPoint can be exactly the index, we're search for OR it can be index-1 OR it can be index+1. So I've added this two additional checks.
Maybe they can be simplified ;-)
The good news: 💯 % tests are green

nitram509 · 2013-04-18T21:41:03Z

I'm curious, what do you think about this approach "using code generation" in general?

pozirk · 2013-04-20T15:34:50Z

I didn't dig too much into the code, but your variant is working 50% faster for me.
Generation of arrays is not a problem and should be done only once.
Just need to check the "ip - country" accuracy, but seems fine for now.
Thank you!

nitram509 · 2013-04-27T16:49:10Z

Uncovered two more bugs: The last and second last IP-range are not correctly detected. I'm working on a fix.

pozirk · 2013-04-27T17:43:16Z

huh, I hope not too many of my players live in these IP-ranges. :)

…ll not be catched

… "Virgin Islands, British" are treated the same

…. "Virgin Island, British")

nitram509 · 2013-04-27T19:52:01Z

Done. I've fixed the two bugs.
In general, the new lookupTests contains 302 unique tests, which give me a better feeling now :-)

I would be glad, if you consider to accept this pull request.

nitram509 added 30 commits October 21, 2012 23:53

renamed test.js to benchmark.js (which is more correct)

60852bc

added a NodeUnit test for proving correctness

b79555d

renamed lookup.js to lookupTest.js

1d5086e

added some hints about nodeunit

0eaa066

cosmetics

1188781

cosmetics

51d7aec

reordered paragraphs

101f943

added IntelliJIdea working files to git ignore list

f1e93b7

made separate test cases for each corner case

cb89a8f

FIX: country name contains a '\r' on windows

FIX: wrong calculation of midpoints

115770b

only reformatting

3f5eaf3

only reformatting and renaming variables for better readability

c3dac81

FIX: lower IP ranges, leftmost and rightmost borders are correctly de…

32ad6cd

…tected, as unit tests are green now

reformatted code, only

24a1d33

renamed variable

a843b2c

generating country maps and lists and saving them to .JS source files

ea898f5

using country-names list with index, which results in half the memory usage

reworked benchmark.js, so it's no more recursive and easier to unders…

e918eb9

…tand

Merge branch 'master' into code_generation

716bdd9

reworked benchmark.js, by using generated sources there's no need to …

6085abd

…wait/warmup

direct array generation speeds up loading of the source file

3a58a3b

direct array generation speeds up loading of the source file

9a4384d

added generate_sources.js as script to build phase within package.json

40099f5

removed warm up test, not needed anymore

ee1918b

fix: don't write the line twice

4ddf392

removed warm up test, not needed anymore

68c9e1f

reworked binary search (see wikipedia) and FIXed last 'red' test

bd05731

merged new binary search algorithm

b9bb72d

Merge branch 'master' into code_generation Conflicts: geoip.js

simplyfied return statement

a356fa8

updated package.json - fixed preinstall hook

1e7969f

shortened the generated code

21fee47

nitram509 added 4 commits March 24, 2013 21:28

fixed example in readme

9021226

fixed example in readme

d5fc3df

cleanups: tools folder for benchmark.js, consistence naming

dbedc8f

fixed require statement in benchmark.js

0ef8603

fixed geoip require statement

7a4f992

fixed binary search, by using Deferred detection of equality approach…

4ee1fe8

… and combining this with more range/equation checks

fixed range check

592baed

nitram509 added 2 commits April 27, 2013 19:08

re-ident to two spaces

c72fe1c

refactored: extracted methods for better readability

d8634ca

nitram509 added 4 commits April 27, 2013 20:15

improved lookupTest, and uncovered a bug: last country in the list wi…

2751af7

…ll not be catched

improved lookupTest, and uncovered one more bug: "Virgin Islands" and…

c74d4df

… "Virgin Islands, British" are treated the same

Fixed problem of commas in country names (example: "Virgin Island" vs…

bfa84f4

…. "Virgin Island, British")

Fixed problem that last records in database weren't resolved correctly

3a21f8c

added one more assertion for ending range

73262d9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More speed and less memory usage via code generation#2

More speed and less memory usage via code generation#2
nitram509 wants to merge 44 commits intobenlowry:masterfrom
nitram509:code_generation

nitram509 commented Nov 4, 2012

Uh oh!

pozirk commented Apr 18, 2013

Uh oh!

pozirk commented Apr 18, 2013

Uh oh!

nitram509 commented Apr 18, 2013

Uh oh!

pozirk commented Apr 18, 2013

Uh oh!

nitram509 commented Apr 18, 2013

Uh oh!

nitram509 commented Apr 18, 2013

Uh oh!

pozirk commented Apr 20, 2013

Uh oh!

nitram509 commented Apr 27, 2013

Uh oh!

pozirk commented Apr 27, 2013

Uh oh!

nitram509 commented Apr 27, 2013

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

nitram509 commented Nov 4, 2012

Uh oh!

pozirk commented Apr 18, 2013

Uh oh!

pozirk commented Apr 18, 2013

Uh oh!

nitram509 commented Apr 18, 2013

Uh oh!

pozirk commented Apr 18, 2013

Uh oh!

nitram509 commented Apr 18, 2013

Uh oh!

nitram509 commented Apr 18, 2013

Uh oh!

pozirk commented Apr 20, 2013

Uh oh!

nitram509 commented Apr 27, 2013

Uh oh!

pozirk commented Apr 27, 2013

Uh oh!

nitram509 commented Apr 27, 2013

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants